-
Notifications
You must be signed in to change notification settings - Fork 3
Add to_string, prints a DDouble in the %e style. #7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mainline
Are you sure you want to change the base?
Conversation
mwallerb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ben,
Thank you for the PR.
However, this need quite a bit of work. Unit tests are missing completely. The formatting is inconsistent with the rest, and I think you should be getting quite an error buildup due to the repeated multiplications with 10.
Please see "Printing Floating-Point Numbers Quickly and Accurately with
Integers" by Florian Loitsch or the "Dragon4" algorithm. I think this is the way to go.
|
|
||
| XPREC_API_EXPORT | ||
| std::string to_string(DDouble d, size_t nDigits){ | ||
| using namespace std; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not import the complete std namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It's confined, there is no namespace pollution.
| bool isnan(DDouble x); | ||
| bool isnormal(DDouble x); | ||
| bool iszero(DDouble x); | ||
| std::string to_string(DDouble d, size_t nDigits = 34); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extending the to_string interface. I am not sure that is what we want to do. I think it is better to respect to use << (which can then be used together with a stringstring).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right.
| //Alright. Get a string started. Handle sign. | ||
| string s; | ||
|
|
||
| if(d.hi() < 0.0) s += "-"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not do this.
There is a clang format file in the repo. You can use clang-format -i src/io.cxx to automatically format the file according to the spect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one-line if's, please split them across lines.
|
|
||
| //Current digit to print is just the leading one. | ||
| auto m = (int)floor(d.hi()); | ||
| assert(m >= 0 && m < 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assertion will occasionally fail due to rounding.
| //Loop over digits, print them. | ||
| nDigits = min(max(nDigits, (size_t)3), (size_t)34); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here?
|
Thanks for the feedback and for pointing me to the other stuff. I'll let you know if I make time to work, it looks like it'd be a rewrite. |
You may or may not want to move this around and/or make it match style better.
Sample use:
Output:
1.414213562373095048801688724209708
Actual value of sqrt(2) to same number of digits:
1.414213562373095048801688724209698